feat(BA-6550): represent model service command as string#12445
Conversation
github-actions
Bot
added
size:L
There was a problem hiding this comment.
Pull request overview
This PR (BA-6550) refactors the model service start_command from an argv list (list[str]) to a single internal command string, while keeping backward compatibility with deprecated list-form input and the legacy command/start_command API fields. Shell handling moves to the kernel: /bin/bash remains the default, but an explicit null/empty shell now triggers direct shlex.split argv execution. An idempotent Alembic data migration converts already-stored model definitions to the new string form across revisions, revision presets, and runtime variant defaults.
Changes:
start_commandis normalized tostr | Noneinternally; lists are folded viashlex.join,{model_path}substitution and arg-appending operate on strings, and the kernel splits/wraps based onshell.shellis widened tostr | None(GQL/DTO/config), with explicit null preserved throughto_resolved; output DTO exposes both newcommandand deprecatedstart_command.- New data migration
ba6615a4d2f1stringifies stored argv lists; extensive test updates across manager/agent/kernel/common.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/common/config.py | Core: start_command str, _resolve_start_command, string-based merge/append/placeholder |
| src/ai/backend/kernel/service.py | Shell None/empty handling: wrap vs shlex.split |
| src/ai/backend/agent/agent.py | Image-cmd fallback and legacy env args use shlex.join |
| src/ai/backend/common/dto/manager/v2/deployment/types.py | Output DTO command + deprecated start_command derivation |
| src/ai/backend/manager/api/adapters/deployment_revision_preset/adapter.py | Maps internal string to command + legacy list |
| .../alembic/versions/ba6615a4d2f1_*.py | Idempotent data migration; # Part of: placeholder unfilled |
| .../gql/deployment/types/revision_preset.py, request.py | shell widened to nullable |
| docs/.../*.graphql, changes/12445.enhance.md | Schema/changelog updates |
| tests/** | Updated expectations to string form; new shell/alias cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
This seems to be a feature job rather than an enhancement or refactoring since API schema changes.
And is this correct?
- API allows array typed
start_commandandshellis used only whenstart_commandis set - Agent and Kernel also allows
start_commandandshellis used only whenstart_commandis set - All Agent, Kernel and API uses
commandif it is set
|
@fregataa Yes, that's correct with a few details:
|
seedspirit
changed the title
| @model_validator(mode="before") | ||
| @classmethod | ||
| def _wrap_str_start_command(cls, data: Any) -> Any: | ||
| return _wrap_str_start_command_into_argv(data) | ||
| def _resolve_start_command(cls, data: Any) -> Any: | ||
| return resolve_model_service_start_command(data) |
There was a problem hiding this comment.
How about changing this model_validator mode to after? then we don't need all data here
There was a problem hiding this comment.
Since this also includes the process of converting currently deprecated input aliases (command, start-command, and list-type start_command) to start_command: str, it seems correct for it to run as a "before" action.
Co-authored-by: octodog <mu001@lablup.com>
Co-authored-by: octodog <mu001@lablup.com>
Summary
start_commandto use a single internal command string while preserving deprecated list input compatibility./bin/bashas the default shell, allow explicit null/empty shell for direct argv execution, and update kernel-side command resolution accordingly.Test plan
pants fmt --changed-since=HEADpants fix --changed-since=HEADpants lint --changed-since=HEADpants check --changed-since=HEADpants test --changed-since=HEADpants lint --changed-since=origin/mainorigin/main..HEAD@603d0918fcommand, deprecated liststart_command, vLLM default model definition, and revision preset storageResolves BA-6550
📚 Documentation preview 📚: https://sorna--12445.org.readthedocs.build/en/12445/
📚 Documentation preview 📚: https://sorna-ko--12445.org.readthedocs.build/ko/12445/